When running clang-tidy on /js/src with the following command: run-clang-tidy-4.0.py -p obj-x86_64-pc-linux-gnu/ -checks=-*,modernize-use-auto,modernize-use-nullptr js/src We find a few suggestions to improve our code by migrating to C++11. Clang-tidy can even apply them automatically with the `-fix` parameter. Let's use this bug to make small improvements to the codebase here and there by keeping a small footprint and limit the review burden. Note: You can install run-clang-tidy-4.0.py on Ubuntu/Debian by following these steps: apt-get install clang-tidy-4.0 # Follow http://apt.llvm.org/ ./mach configure ./mach build-backend --backend=CompileDB ./mach build pre-export ./mach build export run-clang-tidy-4.0.py -p obj-*/ -checks=-*,modernize-use-auto,modernize-use-nullptr js/src

nullptr is great but I'm not sure auto is an improvement when it changes: RegExpFlag flags = RegExpFlag(0); to: auto flags = RegExpFlag(0); Without auto it's obvious |flags| is a RegExpFlag. With auto we don't know, for instance what if RegExpFlag is a function that returns an integer?

(In reply to Jan de Mooij [:jandem] from comment #10) > nullptr is great but I'm not sure auto is an improvement when it changes: > > RegExpFlag flags = RegExpFlag(0); > > to: > > auto flags = RegExpFlag(0); > > Without auto it's obvious |flags| is a RegExpFlag. With auto we don't know, > for instance what if RegExpFlag is a function that returns an integer? Thanks a lot for confirming one of my worries about the auto specifier! We're considering it as part of our static analysis and C++11 conversion efforts, but if we consider `RegExpFlag(0)` to be a false positive for the `modernize-use-auto` checker, we might decide against using it. Out of curiosity, do you think other less ambiguous `auto` conversions could be valuable? E.g.: > - uint32_t n = (uint32_t)dval(di); > + auto n = (uint32_t)dval(di); > - CDataFinalizer::Private* p = (CDataFinalizer::Private*) > - JS_GetPrivate(sourceData); > + auto* p = (CDataFinalizer::Private*)JS_GetPrivate(sourceData); > - NativeObject* nproto = static_cast<NativeObject*>(proto); > + auto* nproto = static_cast<NativeObject*>(proto); > - ImmutablePropertyNamePtr* names = reinterpret_cast<ImmutablePropertyNamePtr*>(commonNames.ref()); > + auto* names = reinterpret_cast<ImmutablePropertyNamePtr*>(commonNames.ref()); etc.

Comment on attachment 8875493 [details] [diff] [review] Use auto specifier in /js/src/builtin/RegExp.cpp. r= Review of attachment 8875493 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/RegExp.cpp @@ +219,5 @@ > return false; > } > > /* Step 3. */ > + auto flags = RegExpFlag(0); This use of `auto` makes the code less explicit / understandable. @@ +456,5 @@ > > // Step 8. > if (args.hasDefined(1)) { > // Step 4.c / 21.2.3.2.2 RegExpInitialize step 4. > + auto flagsArg = RegExpFlag(0); This use of `auto` makes the code less explicit / understandable. @@ +1570,5 @@ > AutoAssertNoPendingException aanpe(cx); > if (!proto->isNative()) > return false; > > + auto* nproto = static_cast<NativeObject*>(proto); This use of `auto` might slightly improve the code by making it shorter.

(In reply to Jan Keromnes [:janx] from comment #11) > Out of curiosity, do you think other less ambiguous `auto` conversions could > be valuable? E.g.: > > - CDataFinalizer::Private* p = (CDataFinalizer::Private*) > > - JS_GetPrivate(sourceData); > > + auto* p = (CDataFinalizer::Private*)JS_GetPrivate(sourceData); Here it could be nice because it's quite a bit shorter. Personally I prefer seeing the explicit types for each of these cases. That's just my preference and maybe it's because I have to get used to |auto|, but if we're considering rewriting things it might be worth asking dev-platform what people think :)

My personal rule of thumb with auto is to use it only when the type is already indicated in the current line, also keeping the pointer/reference annotation (so auto* as in your example). With a few exceptions: declaring iterators over templates structures as auto variables sounds fine to me, to avoid ultra verbose code. (it would make sense indeed to ask dev.platform about this topic)

Thanks a lot for your feedback! I'll close this bug for now, because auto specifiers are mostly a coding style preference, and converting is not always a good idea which makes it hard to automate. We'll disable the "modernize-use-auto" clang-tidy rule for now. If we decide to re-enable it in the future with more useful heuristics, we should ask dev.platform about these heuristics.

Removing leave-open keyword from resolved bugs, per :sylvestre.